AUDIT-28: Backfill pre-existing rows into Envers audit tables to fix "Unable to read" in audit UI#5933
Conversation
Binayak490-cyber
force-pushed
the
AUDIT-28-audit-backfill
branch
3 times, most recently
from
9d093dc to
a2bb48b
Compare
|
Hi @wikumChamith, @druchniewicz, @nsalifu, SonarCloud flagged a few SQL Injection security hotspots due to dynamically constructed queries in These queries only use table and column names sourced from Hibernate/JDBC metadata (not user input). To make this explicit, I added a If an identifier contains any unsafe characters, an Since SonarCloud hotspots require manual review, could a maintainer please review and mark them as Safe if this approach looks correct? |
Binayak490-cyber
force-pushed
the
AUDIT-28-audit-backfill
branch
from
a2bb48b to
6af3ad8
Compare
…'Unable to read' in audit UI
Binayak490-cyber
force-pushed
the
AUDIT-28-audit-backfill
branch
from
6af3ad8 to
fdf4ec8
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5933 +/- ##
============================================
- Coverage 59.31% 59.30% -0.01%
- Complexity 9252 9274 +22
============================================
Files 686 687 +1
Lines 37123 37241 +118
Branches 5452 5475 +23
============================================
+ Hits 22018 22085 +67
- Misses 13141 13178 +37
- Partials 1964 1978 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi @wikumChamith, @druchniewicz, @nsalifu, SonarCloud flagged a few SQL Injection security hotspots due to dynamically constructed queries in EnversAuditTableInitializer. These queries only use table and column names sourced from Hibernate/JDBC metadata (not user input). To make this explicit, I added a requireSafeIdentifier(String) check that validates identifiers against [a-zA-Z_][a-zA-Z0-9_]* before they are used in SQL. If an identifier contains any unsafe characters, an IllegalArgumentException is thrown and the query is not executed. Since SonarCloud hotspots require manual review, could a maintainer please review and mark them as Safe if this approach looks correct? |
|
@Binayak490-cyber did you run your code on real instance of OpenMRS? (real I mean e.g. on your localhost), because when I ran it on my local machine I got multiple errors like It is just an example, it occurs for all audit tables. |
Thanks for testing @druchniewicz ! The fix dynamically discovers the timestamp column from JDBC metadata instead of hardcoding REVTSTMP. Could you test the updated code on your local instance to confirm it resolves the errors? |
Did you test your changes? I think the best will be running clean distro on your local machine and check if your changes work. |
|
@Binayak490-cyber, as @druchniewicz pointed out earlier, your changes don't seem to be working. I can confirm the same. I'm still seeing empty audit tables. Make sure you actually run and manually verify your changes before making a PR. |
…nt revision_entity table
Binayak490-cyber
force-pushed
the
AUDIT-28-audit-backfill
branch
from
8e14571 to
09048e3
Compare
|
Thanks @druchniewicz and @wikumChamith for testing. In the last push, I’ve verified the fixes locally on a fresh OpenMRS distro using Docker + MariaDB. Fixed the On a clean DB, multiple audit tables were successfully backfilled (20+ tables), and no errors were observed in logs. Could you please re-test on your end? |
|
What do you think about using Liquibase CustomTaskChange rather than doing a JDBC backfill? |
Yeah, this. |
|
So, @ibacher, @wikumChamith, @dkayiwa, does it means that I will go for a rework, but before that want to clarify that:
What do you want...? |
We should keep EnversAuditTableInitializer as it is because it needs to run with any schema changes. However, the data backfill is only going to happen once. |
https://github.com/search?q=org%3Aopenmrs%20CustomTaskChange&type=code |
Okk, thanks @wikumChamith!👍 |
Binayak490-cyber
force-pushed
the
AUDIT-28-audit-backfill
branch
from
7e3c22f to
bf75000
Compare
|
|
@wikumChamith, @druchniewicz, @ibacher, @dkayiwa, can you take a look at it now, I have now moved the Envers audit table backfill from a JDBC Hibernate Integrator (runs every startup) into a Liquibase CustomTaskChange (runs exactly once per database), verified locally also and pushed it to the PR...? |
@Binayak490-cyber I'm trying to deploy your changes on my local instance but seems something does not work. I'm using openmrs-core 2.9.x version - from this branch I copied BackfillEnversAuditTablesChangeset.java, EnversAuditTableInitializer.java and your change set to liquibase-update-to-latest-2.9.x (3.0.x xml does not exist in omrs-core 2.9.x) built and deployed it to my local instance. Seems that this migration didn't run at all because I don't see the entry in liquibasechangelog table in database. And audit tables are also empty. Could you verify it? Maybe I miss something? Can you try to run these changes on omrs-core 2.9.x as and check if it works for you? |
@druchniewicz, thanks for testing! I wanted to clarify that this PR targets openmrs-core 3.0.x (master/3.0.0-SNAPSHOT), not 2.9.x. Our changeset is in liquibase-update-to-latest-3.0.x.xml, which only exists in the 3.0.x codebase. The reason the migration didn't run on your 2.9.x instance is that:
Could you test by building and running the full 3.0.x branch with our changes? That is the intended target for this PR. If a 2.9.x backport is needed, that would be a separate effort. |
I'm using 2.9.x because there were some issues with running the latest omrs core snapshot version (3.0.0-snapshot). I used OpenMRS 2.8.3 version and I wanted to test changes related to hibernate envers that were merged recently (in February). When I deployed the latest version of omrs-core from master branch (3.0.0-snapshot) then the run app does not start correctly because of some failing migrations. I needed to back to stable 2.9.x branch (it also contains hibernate envers commit) to run the app successfully. |
Thank you for understanding @druchniewicz. Actually, since the Envers commit is also in 2.9.x, the backfill should work there too. Since this PR is scoping for 3.0.0-SNAPSHOT only, I think it would be cleaner to keep it focused here. Once this gets accepted and merged, I can open a separate PR to add the AUDIT-28-2026-03-19 changeset to liquibase-update-to-latest-2.9.x.xml, rather than mixing both version changes in the same PR which will be very big for a single PR and also there are many tickets and issues in jira related around this, so separately doing all, I think will be good. Regarding the failing migrations on 3.0.0-SNAPSHOT — is that a known issue being tracked separately? Just want to make sure our changes aren't contributing to it. |
@Binayak490-cyber if you have time you can just use the same changes for 2.9.x and check if they works. Just run the refdistro app (use e.g. dev3 tag for docker image), build the omrs-core module from 2.9.x branch and check if audit tables will be backfilled properly. |
@druchniewicz, I tested the changes on 2.9.x using the refdistro app with the dev3 Docker image as you suggested. The backfill changeset works correctly on 2.9.x as well — AUDIT-28-2026-03-19 was recorded in liquibasechangelog and audit tables like concept_name_audit, concept_audit, concept_reference_map_audit were all populated with data. I have ported the changeset to liquibase-update-to-latest-2.9.x.xml and pushed it to my fork. Should I open a separate PR targeting the 2.9.x branch for this as you can also check and test it in that branch...??? |
Yes, please open a separate PR for 2.9.x branch. Other basic tables like Patient, Encounters, Obs etc. have also been populated with all data? |
Thank you @druchniewicz then I will open a separate PR for the 2.9.x branch shortly. Regarding patient_audit, encounter_audit, obs_audit — in my local test I used a fresh database, so those tables were empty simply because there were no existing patients, encounters or observations to backfill. The backfill logic ran correctly for all tables that had data (global_property_audit, privilege_audit, concept_name_audit etc.). To properly verify patient_audit, encounter_audit and obs_audit I would need to test against a database with existing clinical data, which is what you have in your 2.8.3 instance. Would you be able to test the 2.9.x PR once I open it? |
You can simply test it by yourself. Just register a patient, fill in any visit form and you should have data in patient/encounter/obs audit tables. Then you can just remove these audit tables, restart the server and these tables should be created with expected data. |
@druchniewicz, I have now opened PR #5952 targeting the 2.9.x branch with the same backfill changeset ported over with the similar changes as I have done in this PR but this PR is for master 3.0.0-SNAPSHOT only, so you can check it! |
I might be mistaken, but regarding the snapshot version, it seems that we cannot run the OpenMRS Core 3.0.0 snapshot with the Docker-based Reference Application. From what I understand, deployments work only up to Core 2.9.x. The likely reason is the Tomcat version mismatch. The snapshot version uses Tomcat 10 or above, while the Reference Application Docker setup appears to be using a Tomcat version below 10. Because of this, it probably only supports Core up to 2.9.x. Also, even when I tried running the snapshot version manually on a local Tomcat setup, the audit web module did not run smoothly. Just to confirm, have you tested all these changes in a real application environment using the snapshot version along with the existing audit web module? I’m asking because I tried running the audit web module with the current snapshot, but it did not work. |
@sudhanshu-raj, you are correct but not fully about that the 3.0.0-SNAPSHOT uses Tomcat 10+ (Jakarta EE) while the refdistro Docker image uses Tomcat 9 — this is a known compatibility issue. For local testing of the 3.0.x changes I used a custom Docker setup with tomcat:10.1-jdk21 as the base image and replaced the WAR directly, which allowed the backfill to be verified successfully — audit tables like privilege_audit, global_property_audit, concept_name_audit were all populated correctly. Regarding the audit web module not working on the snapshot — that is a separate issue from what this PR addresses. This PR only fixes the backfill of pre-existing data into audit tables at the core level. The audit web module compatibility with 3.0.x is outside the scope of this PR. For 2.9.x (PR #5952), the refdistro dev3 Docker image works correctly since it runs Tomcat 9, and the backfill was verified there as well. |
@Binayak490-cyber I fetched your PR and deployed it on my local machine and seems that this migration is not executed at all. I'm 100% sure that openmrs.war with your changes is deployed on my local machine properly. I don't see the appropriate entry in liquibasechangelog table |
@druchniewicz, have you tried this PR #5952, this is the real PR for 2.9.x branch...! |
#5952 (comment), @druchniewicz, please go for my this comment....! |
6 participants
Summary
Fixes AUDIT-28 where important fields appear as "Unable to read" in the audit UI.
Root Cause
When Envers auditing is enabled after data already exists, the *_audit tables are created empty. New audit records referencing those pre-existing entities cannot be resolved by Envers.
Fix
Added backfillAuditTables() in EnversAuditTableInitializer to populate audit tables with existing rows during initialization. Missing rows are inserted with REVTYPE=0 (ADD) under a synthetic revision.
Changes
Testing
Verified using H2 and unit tests that: